Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Error and SetError to all datasets. #152

Merged
merged 4 commits into from
Sep 6, 2019
Merged

Add Error and SetError to all datasets. #152

merged 4 commits into from
Sep 6, 2019

Conversation

marshall-mcmullen
Copy link
Contributor

This addresses #150 by
adding SetError and Error methods to each dataset. This will allow
end-user query building (e.g. users of goqu) to be able to mark a
dataset as being in a failed state via SetError and not have to track
that separately.

It only sets the error if one has not already been set. It also sets it
on the underlying SQLBuilder so that it too will know about the error
and not try to proceed with any futher query building. As such, future
calls to ToSQL will return the error as well.

This addresses #150 by
adding SetError and Error methods to each dataset. This will allow
end-user query building (e.g. users of goqu) to be able to mark a
dataset as being in a failed state via SetError and not have to track
that separately.

It only sets the error if one has not already been set. It also sets it
on the underlying SQLBuilder so that it too will know about the error
and not try to proceed with any futher query building. As such, future
calls to ToSQL will return the error as well.
@marshall-mcmullen marshall-mcmullen mentioned this pull request Sep 5, 2019
@doug-martin
Copy link
Owner

Quick turnaround! My only ask is to add some docs around this and why one might use it, as I think quite a few users of this library may find this useful as it could really streamline some code that is focused on dynamically building queries. Great Job!

@marshall-mcmullen
Copy link
Contributor Author

I'll add some documentation around this tonight... Thanks!

@marshall-mcmullen
Copy link
Contributor Author

Hey @doug-martin check out the latest commit and see what you think. I added some documentation and examples to each dataset's docs. Let me know if you want something changed or explained differently.

var ds = goqu.Delete("test").
Where(goqu.C(name).Eq(value))

if len(name) == 0 {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is pedantic but I think it makes sense to put the validation before setting the where. In the end it wont make a difference based on the implementation; but setting the error before creating the clause signifies intention.

I often struggle with showing intention vs how its actually implemented, but in this case validation should come first.

If you disagree let me know, but I want docs to demonstrate how it should be used and not necessarily how it could be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally agree. I didn't spend a ton of time thinking about the examples from that perspective but you're totally right. I'll push an update. Let me know if you find other things that are confusing.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for being so receptive to comments! I think the same applies for the rest of the examples (they all seem to be based off of this example). Should be a quick update that will helps many users in the long run.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once you update Ill merge update history and release!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely! Yes, I copy-pasted this example to the others and made edits where necessary.

I'll fix the other examples too.

@doug-martin doug-martin closed this Sep 5, 2019
@doug-martin doug-martin reopened this Sep 5, 2019
@doug-martin doug-martin merged commit 26b887b into doug-martin:master Sep 6, 2019
doug-martin added a commit that referenced this pull request Sep 6, 2019
* [ADDED] `SetError()` and `Error()` to all datasets. #152 and #150 - @marshallmcmullen
@doug-martin doug-martin mentioned this pull request Sep 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants